use getTerminatorOrNull for Windows EH with LLVM23#5121
Conversation
There was a problem hiding this comment.
Pull request overview
This PR applies small source-level updates to restore/build compatibility with current LLVM APIs and tighten up a control-flow cloning edge case in the MS C++ EH helper code.
Changes:
- Update
cloneBlocks()to guard successor remapping usinggetNumSuccessors()before callinggetSuccessor(0). - Switch label prefix emission to
MCAsmInfo::getPrivateLabelPrefix()when printing inline-asm label names. - Update the
opts::setFunctionAttributes()wrapper to match the newerllvm::codegen::setFunctionAttributes()parameter ordering/signature.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| gen/ms-cxx-helper.cpp | Adds successor-count guarding when remapping the terminal branch target during block cloning. |
| gen/llvmhelpers.cpp | Uses getPrivateLabelPrefix() for label-name emission. |
| driver/cl_options-llvm.cpp | Updates wrapper call to llvm::codegen::setFunctionAttributes() to match the current signature. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Where did you hit this problem - when building druntime/Phobos already, or for an unrelated code base? In the first case, it might be an LLVM trunk regression/change (e.g., not returning null if there aren't any successors - which could still be fixed for LLVM 23, it wasn't even branched off yet). In the 2nd case, a testcase is needed. |
|
When building druntime/phobos. Alas I don't have a windows system, I'll ask on the LLVM discord and see what the consensus is. |
|
|
Oof; thx for asking. We have more usages incl. null checks. |
thewilsonator
changed the title
getTerminatorOrNull for Windows EH with LLVM23
thewilsonator
left a comment
thewilsonator
left a comment
There was a problem hiding this comment.
Otherwise looks great.
kinke
left a comment
kinke
left a comment
There was a problem hiding this comment.
As said, there are more usages.
… across all usages
| #if LLVM_VERSION_MAJOR >= 23 | ||
| return bb->getTerminatorOrNull(); | ||
| #else | ||
| return bb->getTerminator(); | ||
| #endif |
There was a problem hiding this comment.
this one is not needed to be updated because it is in the #else branch of #if LLVM_VERSION_MAJOR >= 19
There was a problem hiding this comment.
tks for clarifying
thewilsonator
dismissed
kinke’s stale review
All other usages updated, except for the one in the else branch of #if LLVM_VERSION_MAJPR >= 19
| #else | ||
| auto *Term = B->getTerminator(); | ||
| #endif | ||
| unsigned SuccCount = Term->getNumSuccessors(); |
There was a problem hiding this comment.
Here (and in other sites) the breaking change for the existing API would have actually made sense, because we don't check for null afterwards, assuming that LLVM trunk introduced an appropriate assertion.
This PR contains two related fixes for building and running LDC with LLVM trunk on Windows:
1. MSVC Build Fix:
cl_options-llvm.cppandllvmhelpers.cppto ensure compatibility with LLVM trunk.2. cloneBlocks Segfault Fix:
0xC0000005Access Violation incloneBlockswhen compiling code withtry-finallycleanups.term->isTerminator()inms-cxx-helper.cppto prevent crashing when the frontend generates a basic block without a proper terminator (e.g., ending abruptly with acallto__dtor).